-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[browser][file system] GetLogicalDrives implementation #39763
Conversation
- Directory.GetLogicalDrives threw PNSE. Follows existing code paths. - Add common DriveInfoInternal.Browser that is common code path for other implementations - Environment.GetLogicalDrives - DriveInfo.Drives Fix for FileSystemTest https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/tests/Directory/GetLogicalDrives.cs
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@@ -15,6 +15,6 @@ public sealed partial class DriveInfo | |||
public long TotalFreeSpace => 0; | |||
public long TotalSize => 0; | |||
|
|||
private static string[] GetMountPoints() => Environment.GetLogicalDrives(); | |||
private static string[] GetMountPoints() => DriveInfoInternal.GetLogicalDrives(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change required since Environment.GetLogicalDrives
returns the same value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No not required but removes an extra call because Environment.GetLogicalDrives()
calls DriveInfoInternal.GetLogicalDrives();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this extra call matter in this case?
This is adding a second copy of DriveInfoInternal.Browser.cs that is much worse for Browser in particular than an extra call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas not sure what you mean, this follows the existing pattern for DriveInfoInternal.Unix.cs
that is already there.
The extra call doesn't really matter but I see no harm in getting rid of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When having a choice, I think we should be optimizing Browser for smaller footprint, not for tiny bit better throughput of APIs that are slow by design and are very unlikely to be used in Browser.
I believe that the extra copy of DriveInfoInternal.Browser.cs
is causing more harm here for browser than the extra call.
This does not matter much since the difference is small and all this code is going to be removed by the linker for browser anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, you meant the extra copy of the implementation that is compiled into the library, I thought you meant some duplicate file :)
@@ -15,6 +15,6 @@ public sealed partial class DriveInfo | |||
public long TotalFreeSpace => 0; | |||
public long TotalSize => 0; | |||
|
|||
private static string[] GetMountPoints() => Environment.GetLogicalDrives(); | |||
private static string[] GetMountPoints() => DriveInfoInternal.GetLogicalDrives(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this extra call matter in this case?
This is adding a second copy of DriveInfoInternal.Browser.cs that is much worse for Browser in particular than an extra call.
Addressed in recent commits |
The wasm test failure was due to #39473 |
- Directory.GetLogicalDrives threw PNSE. Follows existing code paths. - Add common DriveInfoInternal.Browser that is common code path for other implementations - Environment.GetLogicalDrives - DriveInfo.Drives Fix for FileSystemTest https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/tests/Directory/GetLogicalDrives.cs
Fix for FileSystemTest https://github.com/dotnet/runtime/blob/master/src/libraries/System.IO.FileSystem/tests/Directory/GetLogicalDrives.cs